-
Notifications
You must be signed in to change notification settings - Fork 363
Add if then else schema validation #715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add if then else schema validation #715
Conversation
Any plans merging this PR? |
Short answer: Not yet. Long answer: We've been putting effort in reviving this repository, which meant a lot of cleanup, triage en updating to current PHP versions. At this point we are working on those items. Adding additional drafts is definitely our goal but not our priority at this point in time. |
+1 I would love this functionality ❤️ |
// minProperties and maxProperties constraints only applies on objects elements. | ||
if (!is_object($element)) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary anymore. The library already has this validation.
const ALWAYS_FAILS = 'alwaysFails'; | ||
const ANY_OF = 'anyOf'; | ||
const CONDITIONAL_IF = 'if'; | ||
const CONDITIONAL_THEN = 'then'; | ||
const CONDITIONAL_ELSE = 'else'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constants are public. The visibility has been added in the library. It should be added in this fix as-well.
], | ||
"require": { | ||
"php": ">=5.3.3", | ||
"ext-json": "*", | ||
"marc-mabe/php-enum":"^2.0 || ^3.0 || ^4.0", | ||
"icecave/parity": "1.0.0" | ||
}, | ||
"require-dev": { | ||
"friendsofphp/php-cs-fixer": "~2.2.20 || ~2.19.0", | ||
"json-schema/json-schema-test-suite": "1.2.0", | ||
"json-schema/json-schema-test-suite": "2.0.0", | ||
"phpunit/phpunit": "^4.8.35" | ||
}, | ||
"extra": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes are not needed since it has been done in the master branch
@Norda-AD thank you for all the feedback on the PR. In all honestly I’m not sure if the PR in this form will ever get merged. I really like your efforts but also don’t want to disappoint about the PR never making it into main( currently still called master) The difficulty lies in only adding if then else logic in the current flow. With draft3 and 4 being very alike adding support for the if then else would break those drafts as it would add more keywords then documented. Thinking ahead I would like to add support for more drafts (including the one that has the if then else support) like this PR which is currently pending for Draft 6 |
@DannyvdSluijs Thank you for your feedback. I started digging into this PR since I need this to validate my own code. I understand that you have other priorities. In the meantime, I am trying to decorate the class to fit my needs. I am currently working on adapting this PR to the new code. Basically fixing my comments. |
@Norda-AD I've just merged and released the draft-6 support. So this makes draft-7 next. I'll close this PR for now as I'll be opening a draft 7 PR in the upcoming days. Your feedback has been very helpful in this PR. I'll use it when doing the fresh draft-7 PR. Thanks for your help. |
@DannyvdSluijs Can't wait to see it. Thank you for the feedback! |
You can find it under #847 |
Replay of #581